fix(lambda-nodejs): copy .npmrc for pnpm to enable private registry authentication #36587
Conversation
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request This fix modifies the Docker bundling command to copy The fix is covered by a unit test that verifies the bundling command includes |
aws-cdk-automation
added
the
pr-linter/exemption-requested
PR Update: Changed approach from file copy to base64 encodingThe initial implementation failed the integration test because it tried to copy New approach
This works because:
Testing
|
aws-cdk-automation
added
the
pr/needs-community-review
| }); | ||
| }); | ||
|
|
||
| test('pnpm with nodeModules writes .npmrc using base64 for private registry authentication', () => { |
There was a problem hiding this comment.
Nit. Can you please add coverage for the win32 platform?
There was a problem hiding this comment.
Thanks! Added and re-based.
…uthentication When using nodeModules with pnpm, CDK creates a pnpm-workspace.yaml which prevents pnpm from inheriting .npmrc from parent directories. This fix copies the project's .npmrc to the bundling directory when it exists. Fixes aws#36567
sebastianplesciuc
force-pushed
the
sebastianplesciuc/fix-lambda-nodejs-pnpm-npmrc
branch
from
76734a4 to
9eba1b9
Compare
| assetHashType: AssetHashType.OUTPUT, | ||
| bundling: expect.objectContaining({ | ||
| command: expect.arrayContaining([ | ||
| expect.stringContaining(`echo '${npmrcBase64}' | base64 -d`), |
There was a problem hiding this comment.
Weird. I'd expect this to be powershell -Command .... I suspect we should
Bundling.bundle(stack, { platform: 'win32', ... });Instead of spyOn(os, 'platform')?
Am I missing something?
There was a problem hiding this comment.
No, it's completely my fault for rushing this and trying to copy the test from 158. I'll try and fix this today. Thanks for catching it.
There was a problem hiding this comment.
Okay. So digging into this, the BundlingProps interface doesn't currently expose a platform option - the platform is detected at runtime via os.platform(). The existing win32 test ("esbuild with Windows paths") uses the same jest.spyOn(os, 'platform') pattern.
The reason I missed the bash thing is because only local bundling on Windows will run the PowerShell command, in Docker it will always be bash. I'll try and make a cheeky test by calling tryBundle() directly to force local bundling and mock spawnSync to capture the cmd /c call.
lpizzinidev
left a comment
lpizzinidev
left a comment
There was a problem hiding this comment.
Thanks for following up!
3 participants
Issue # 36567
Closes #36567
Reason for this change
When using nodeModules with pnpm, CDK creates a
pnpm-workspace.yamlin the bundling directory (added in #21911). This causes pnpm to treat the bundling directory as a standalone workspace root, which prevents it from inheriting .npmrc configuration from parent directories. This breaks authentication for packages from private registries.Description of changes
When pnpm is detected and
nodeModulesis specified, copy the project's.npmrc(if it exists) to the bundling directory alongsidepnpm-workspace.yaml. This allows pnpm to find auth tokens for private registries.Describe any new or updated permissions being added
N/A
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license